[canvas] Create Expression Factories; remove legacy search service #107353
Conversation
clintandrewhall
added
review
Team:Presentation
|
Pinging @elastic/kibana-presentation (Team:Presentation) |
clintandrewhall
force-pushed
the
canvas/search-service
branch
from
675302e to
8980de4
Compare
clintandrewhall
changed the title
clintandrewhall
force-pushed
the
canvas/search-service
branch
3 times, most recently
from
9dc2025 to
c59683a
Compare
clintandrewhall
added
the
auto-backport
clintandrewhall
force-pushed
the
canvas/search-service
branch
2 times, most recently
from
1908cf9 to
807604b
Compare
| @@ -37,9 +40,10 @@ export class CanvasSrcPlugin implements Plugin<void, void, SetupDeps, StartDeps> | |||
|
|
|||
| plugins.canvas.addRenderers(renderFunctions); | |||
|
|
|||
| core.getStartServices().then(([coreStart, depsStart]) => { | |||
| core.getStartServices().then(([coreStart, startPlugins]) => { | |||
There was a problem hiding this comment.
this doesn't look right. you await core.getStartServices() here which will only resolve once the start lifectcle completes. this means that your setup lifecycle actually completes after start.
what you should rather do is pass core.getStartServices method to your factory and await on it inside your function body. this way the setup lifecycle will be done in time as we'll only await on start services when running the function (which can only happen after start lifecycle)
also its not clear to me if this functions are reistered on both client and server (they should be)
take a look at how data plugin does it for example:
here we are registering a function and passing core.getStartServices to the factory
here and here you can see how we wrap core services so we can pass a common thing to the function on client and server. note that this is still just async function being passed to actual kibana_context factory (so nothing executed/awaited for at setup time)
here you can see that how we await on core.getStartServices inside function body of kibana_context
There was a problem hiding this comment.
So I've been looking this over, and this feedback is entirely valid and really important, but unfortunately I don't think it can be addressed in this PR. We will most certainly address it in #105675.
@ppisljar My choices are to commit as-is, or fix this one case the correct way and leave the others. How do you feel about me committing this to match the current (admittedly incorrect) pattern and fix all of this in #105675? Otherwise this legacy service is going to have to stick around until we address #105675.
There was a problem hiding this comment.
I am fine leaving this as it is in this PR, we are not changing it for worse and if you think you are gonna reach the end goal easier in steps that is completely ok with me.
clintandrewhall
force-pushed
the
canvas/search-service
branch
from
5a7cc36 to
40f319d
Compare
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
Page load bundle
Unknown metric groupsAPI count
API count missing comments
Non-exported public API item count
History
To update your PR or re-run it, just comment with: |
| @@ -37,9 +40,10 @@ export class CanvasSrcPlugin implements Plugin<void, void, SetupDeps, StartDeps> | |||
|
|
|||
| plugins.canvas.addRenderers(renderFunctions); | |||
|
|
|||
| core.getStartServices().then(([coreStart, depsStart]) => { | |||
| core.getStartServices().then(([coreStart, startPlugins]) => { | |||
| plugins.canvas.addFunctionFactories(factories, { coreStart, startPlugins }); | |||
There was a problem hiding this comment.
This abstraction feels to general to me and not general enough on the other hand.
We are giving whole coreStart and all plugins start contracts to each factory. Most factories need a single thing. And possibly it could be something that is not provided on any start contract (but something internal to the plugin providing the function). When you will try to use this code on the server you will have a lot of problems as server and client side start contracts don't look the same.
I think we should rather call each factory here and provide it just what it needs.
plugins.canvas.registerFunction(esdocsFactory({ search: startPlugins.data.search) });
this will make it clearer what the dependencies are, it will make it easier to make code available on the server (you will only need to abstract search behind a common server/client interface) and it gives flexibility for whatever random thing you need to provide to some other functions in the future.
Summary
This PR creates a new Expressions Function Definition Factory, so plugin lifecycle dependencies can be provided to the function definitions when they are added to the Expressions plugin, rather than accessing a module-level singleton.
Since the only dependencies on the legacy search service were these functions, I've also deleted the legacy service.
This also reconfigures our
i18nto co-locate the strings, introduces a new Factory type (that might be interest to @elastic/kibana-app-services and @ppisljar), and alters the Canvas Plugin API to include a setter.What is this?
In #88112 the Presentation Team created a Service Abstraction API which allows any of our solutions to uniformly (and safely) create abstractions between external services and the means by which we use them. It also provides a standard method for "swapping" service implementations at "start" time, (e.g. Kibana setup/start, or Storybook start, or Jest setup, etc). That means, for example, Storybook Controls can give arguments to the services to adjust mocks on-the-fly from the Storybook application.
Canvas already had a rough service abstraction layer that was a bit rigid, but was also unsafe: the singleton providing services was defined as a module-level constant. This means it exists and can be accessed outside the setup/start lifecycle of a Kibana plugin. 😞
This forthcoming collection of PRs moves each individual service abstraction to our "official" API and removes the legacy service. This makes it easier to review and test each service.